linters: add forbiddenmarkers and nonullable linters #111
Conversation
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
|
I'll need to add the linter documentation to the new location as well :P /hold |
k8s-ci-robot
added
the
do-not-merge/hold
| const name = "forbiddenmarkers" | ||
|
|
||
| type analyzer struct { | ||
| forbiddenMarkers []string |
There was a problem hiding this comment.
What if I wanted to forbid a marker only if it had a certain parameter? Or a certain value?
E.g. I want to prevent someone from using a particular kubebuilder:validation:Format, would this allow me to do that?
Or I wanted to say, you can't use the optionalOldSelf property on an XValidation?
There was a problem hiding this comment.
Good questions. This would not allow that for now. I think those are reasonable use cases and make sense to consider.
I can add some configuration options to allow for this type of customization.
pkg/analysis/nonullable/analyzer.go
Outdated
| analyzer.Name = name | ||
| analyzer.Doc = "Check that nullable marker is not present on any types or fields." |
There was a problem hiding this comment.
Maybe create a constructor on the forbidden markers side that allows you to pass options for Name and Doc, rather than overriding them here? Then this just becomes
return NewAnalyzerWithOpts(forbidden.Opts{
Name: ...
Doc: ...
Markers: []string{...}
})
Or even, you could just call that inside the initializer.
| * The `nonullable` linter ensures that types and fields do not have the `nullable` marker. | ||
| * | ||
| * Fixes are suggested to remove the `nullable` marker. | ||
| * |
There was a problem hiding this comment.
Why the asterisks at the beginning of each line?
There was a problem hiding this comment.
Copied format from another linter doc.go - but also I think my editor does this by default. I can strip the asterisks at the beginning of each line if necessary
There was a problem hiding this comment.
Doesn't matter to me, but normally with /* */ syntax I didn't think it was needed
| // Initializer returns the AnalyzerInitializer for this | ||
| // Analyzer so that it can be added to the registry. | ||
| func Initializer() initializer { | ||
| return initializer{} | ||
| } | ||
|
|
||
| // intializer implements the AnalyzerInitializer interface. | ||
| type initializer struct{} | ||
|
|
||
| // Name returns the name of the Analyzer. | ||
| func (initializer) Name() string { | ||
| return name | ||
| } | ||
|
|
||
| // Init returns the intialized Analyzer. | ||
| func (initializer) Init(_ config.LintersConfig) (*analysis.Analyzer, error) { | ||
| return newAnalyzer(), nil | ||
| } | ||
|
|
||
| // Default determines whether this Analyzer is on by default, or not. | ||
| func (initializer) Default() bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I'm starting to wonder if we should create a util to generate the initializer from a func 🤔
There was a problem hiding this comment.
I like that idea, but to keep the scope of this PR tightened to these two linters I'd suggest that be a follow up.
There was a problem hiding this comment.
This is now implemented, PR will need a rebase
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
k8s-ci-robot
added
size/XL
|
Working on allowing configuration of forbidden attributes and values. /hold |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
needs-rebase
Adds a disabled by default
forbiddenmarkerslinter that has configuration options to specify marker identifiers that should never exist on a type/field. Suggests fixes to remove the forbidden markers.Adds an enabled by default
nonullablelinter that wraps theforbiddenmarkerslinter and configures thenullablemarker to be forbidden.Fixes #100